Skip to content

Conversation

@squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Nov 13, 2025

Description

#3900 introduced a way to show the volume fees to users in advance via quotes. Autopilot should start applying the same policies simultaneously to avoid situations where the user receives an incorrect quote. This PR introduces timestamp-based configs in both orderbook and autopilot crates that control when the volume fee should start applying.

For the orderbook, it is pretty straightforward, which adds an optional timestamp param to the existing volume fee factor config, and each time the service tries to apply volume fees, it checks for the current time. If the timestamp config is None, it means volume fees are applied unconditionally, which should be useful once switched to a long-lasting config.

The autopilot config is a bit more sophisticated. It introduces a separate "upcoming" fee policies config with the effective "from" timestamp. So, if another order's creation timestamp is after the configured upcoming fee policy timestamp, the service starts using this fee policy. This is useful because the volume fee factor affects price improvement and surplus fee policy caps, so each time the volume fee factor is updated, other fee policy configs need to be adjusted accordingly, so we need to switch to the new fee policies set altogether. The config is also optional and can be easily switched to permanent.

The major disadvantage of this approach is that orderbook and autopilot use configs from different sources. A more correct approach would be to use a shared config via a DB or similar, but this would require many more changes, and we should probably avoid any mistakes by making deeper reviews.

How to test

New e2e tests.

@squadgazzz squadgazzz force-pushed the protocol-fees-timestamp branch from 919126f to 5415644 Compare November 13, 2025 21:07
@squadgazzz squadgazzz marked this pull request as ready for review November 14, 2025 09:32
@squadgazzz squadgazzz requested a review from a team as a code owner November 14, 2025 09:32
Copilot AI review requested due to automatic review settings November 14, 2025 09:32
Copilot finished reviewing on behalf of squadgazzz November 14, 2025 09:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces timestamp-based configuration for volume fee policies to ensure synchronization between the orderbook and autopilot services. The changes enable both services to start applying volume fees at the same configured time, preventing users from receiving incorrect quotes.

  • Added optional effective_from_timestamp to volume fee configuration in orderbook
  • Introduced "upcoming" fee policies configuration in autopilot with effective timestamp
  • Both services now check timestamps before applying volume fees

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/orderbook/src/run.rs Updated to pass the new volume_fee_config structure instead of simple fee factor
crates/orderbook/src/quoter.rs Added timestamp checking logic to only apply volume fees when effective timestamp is reached, updated tests
crates/orderbook/src/arguments.rs Refactored volume fee configuration into a VolumeFeeConfig struct with optional effective_from_timestamp field
crates/e2e/tests/e2e/quoting.rs Updated test to use new command-line arguments with future timestamp to verify fee is not applied
crates/e2e/tests/e2e/protocol_fee.rs Added new test for upcoming future fee policies and updated existing test to use new configuration structure
crates/e2e/tests/e2e/limit_orders.rs Migrated from to_string() to into_args() method for protocol fee configuration
crates/e2e/src/setup/fee.rs Restructured ProtocolFeesConfig to support upcoming fee policies and converted Display implementation to into_args() method
crates/autopilot/src/run.rs Updated to pass the full fee policies config structure instead of individual parameters
crates/autopilot/src/domain/fee/mod.rs Added logic to select between current and upcoming fee policies based on order creation timestamp
crates/autopilot/src/arguments.rs Introduced FeePoliciesConfig and UpcomingFeePolicies structures to support timestamp-based fee policy switching

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# Conflicts:
#	crates/e2e/tests/e2e/quoting.rs
#	crates/orderbook/src/arguments.rs
#	crates/orderbook/src/run.rs
Copy link
Contributor

@m-sz m-sz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for extensively covering the change by well documented E2E tests!


let order = services.get_order(&uid).await.unwrap();
let fee_in_buy_token = quote.fee_amount * quote.buy_amount / quote.sell_amount;
assert!(order.metadata.executed_fee >= fee_in_buy_token + quote.sell_amount / 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 10 comes from effective Volume Fee Policy set to 0.1 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is just a copy of the previous buy order, with a single tiny configuration difference.

Comment on lines +57 to +60
pub struct UpcomingProtocolFees {
fee_policies: Vec<ProtocolFee>,
effective_from_timestamp: DateTime<Utc>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we handle the upcoming/general protocol fees uniformly, by having the "always present" fees just start at UNIX epoch? This would simplify the approach, although will add timestamp checking to all fees in general. Let's discuss :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't get your comment. Could you rephrase?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ProtocolFees struct:

pub struct ProtocolFees {
    fee_policies: Vec<ProtocolFee>,
    max_partner_fee: FeeFactor,
    upcoming_fee_policies: Option<UpcomingProtocolFees>,
}

contains fee_policies that are always applied unless superseded by upcoming_fee_policies which are essentially the same, but with an activation timestamp.

I was suggesting to unify these two, and instead support the generally applicable and late activated protocol fees in the same way. This would require the fee_policies to be a Vec of UpcomingProtocolFees.

But thinking about it the second time, I think it would unnecessarily complicate the solution as currently we support only 1 instance of upcoming protocol fees, whereas the generalization would have to support an arbitrary number of upcoming protocol fees. Then the evaluation would require making sure We apply the correct protocol fee based on the current time.

I am happy to approve the PR as-is.

@fafk
Copy link
Contributor

fafk commented Nov 14, 2025

So, if another order's creation timestamp is before the configured upcoming fee policy timestamp, the service starts using this fee policy.

Do you mean after the configured upcoming fee policy?

@squadgazzz
Copy link
Contributor Author

Do you mean after the configured upcoming fee policy?

Yes, my bad.

Copy link
Contributor

@m-sz m-sz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +57 to +60
pub struct UpcomingProtocolFees {
fee_policies: Vec<ProtocolFee>,
effective_from_timestamp: DateTime<Utc>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ProtocolFees struct:

pub struct ProtocolFees {
    fee_policies: Vec<ProtocolFee>,
    max_partner_fee: FeeFactor,
    upcoming_fee_policies: Option<UpcomingProtocolFees>,
}

contains fee_policies that are always applied unless superseded by upcoming_fee_policies which are essentially the same, but with an activation timestamp.

I was suggesting to unify these two, and instead support the generally applicable and late activated protocol fees in the same way. This would require the fee_policies to be a Vec of UpcomingProtocolFees.

But thinking about it the second time, I think it would unnecessarily complicate the solution as currently we support only 1 instance of upcoming protocol fees, whereas the generalization would have to support an arbitrary number of upcoming protocol fees. Then the evaluation would require making sure We apply the correct protocol fee based on the current time.

I am happy to approve the PR as-is.

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the PR looks good (thanks for adding a test!). I think the semantics we want though is that even orders that are already in the orderbook should start carrying the new fee policies.

which should be useful once switched to a long-lasting config.

I would hope we remove this config once it's permanent.

#[clap(long, env, default_value = "0.01")]
pub fee_policy_max_partner_fee: FeeFactor,
#[clap(flatten)]
pub fee_policies_config: FeePoliciesConfig,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice refactoring!

) -> domain::Order {
let protocol_fees = self
.fee_policies
// Use new fee policies if the order creation date is after their effective
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the desired logic. As of the target date, all orders that are settled should be settled according to the new fee policies (as these are what decides the rewards that solvers will receive for these trades0.

In particular, a limit order that has been placed in the past should also carry the new fee policy going forward.

I'm not sure what the best way to achieve this is, but don't we build the fee policies from scratch on every auction? In this case I'd apply the logic like in the orderbook - as of a certain timestamp we start adding them unconditionally on when the order was placed.

}

#[test]
fn test_ignore_volume_fees_before_effective_date() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it also test the other case so we know the timestamp flow works?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants